fix(pipeline): suppress weak short-name matches for Go selector calls - #1907
fix(pipeline): suppress weak short-name matches for Go selector calls#1907ilyabrykau-orca wants to merge 1 commit into
Conversation
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
3f1a325 to
7f4f68c
Compare
|
Rebased onto current main (post-#1903). #1903 generalized the TS/JS guard into |
|
Retriggered CI with a no-op amend (identical tree, new SHA — I lack rerun rights on this repo). The two failures on the previous run look environmental, not change-related: |
7f4f68c to
f76971f
Compare
The READS/WRITES resolvers and the USAGE registry fallback hand bare reference text to the short-name registry, which contains Field nodes - so once Go struct fields exist (DeusData#1935), every Go local err := ... binds whichever struct field is named err, project-wide: 21308 USAGE and 5191 WRITES onto Go fields on the measured repo, top target a test struct's field T collecting 3013 edges. In Go that binding is impossible by construction: a field is only reachable through a selector expression (x.f), and selector references resolve on the LSP path - every Field-targeted reference edge in the census carried dot-less text. Add cbm_go_suppress_bare_field_ref() next to the DeusData#1928 predicate and consult it at the same four sites (both READS/WRITES resolvers, both USAGE registry fallbacks): drop the bind when the file is Go, the target label is Field, and the reference text has no '.'. Go-gated because C#/Java/C++/Python method bodies legitimately reference their own members bare (cp_reads_writes_cs_static_field pins that shape). Field-validated on the DeusData#1940 stack: USAGE onto Go fields 21308 -> 0, WRITES 5191 -> 0; the only remaining field-targeted edges are 2466 CALLS, which are DeusData#1906/DeusData#1907's selector-guard territory. Reproduce- first pipeline probes (sequential + parallel twins) were RED on the stack without this commit. Fixes DeusData#1942 Signed-off-by: Ilya Brykau <ilya.brykau@orca.security>
The READS/WRITES resolvers and the USAGE registry fallback hand bare reference text to the short-name registry, which contains Field nodes - so once Go struct fields exist (DeusData#1935), every Go local err := ... binds whichever struct field is named err, project-wide: 21308 USAGE and 5191 WRITES onto Go fields on the measured repo, top target a test struct's field T collecting 3013 edges. In Go that binding is impossible by construction: a field is only reachable through a selector expression (x.f), and selector references resolve on the LSP path - every Field-targeted reference edge in the census carried dot-less text. Add cbm_go_suppress_bare_field_ref() next to the DeusData#1928 predicate and consult it at the same four sites (both READS/WRITES resolvers, both USAGE registry fallbacks): drop the bind when the file is Go, the target label is Field, and the reference text has no '.'. Go-gated because C#/Java/C++/Python method bodies legitimately reference their own members bare (cp_reads_writes_cs_static_field pins that shape). Field-validated on the DeusData#1940 stack: USAGE onto Go fields 21308 -> 0, WRITES 5191 -> 0; the only remaining field-targeted edges are 2466 CALLS, which are DeusData#1906/DeusData#1907's selector-guard territory. Reproduce- first pipeline probes (sequential + parallel twins) were RED on the stack without this commit. Fixes DeusData#1942 Signed-off-by: Ilya Brykau <ilya.brykau@orca.security>
The READS/WRITES resolvers and the USAGE registry fallback hand bare reference text to the short-name registry, which contains Field nodes - so once Go struct fields exist (DeusData#1935), every Go local err := ... binds whichever struct field is named err, project-wide: 21308 USAGE and 5191 WRITES onto Go fields on the measured repo, top target a test struct's field T collecting 3013 edges. In Go that binding is impossible by construction: a field is only reachable through a selector expression (x.f), and selector references resolve on the LSP path - every Field-targeted reference edge in the census carried dot-less text. Add cbm_go_suppress_bare_field_ref() next to the DeusData#1928 predicate and consult it at the same four sites (both READS/WRITES resolvers, both USAGE registry fallbacks): drop the bind when the file is Go, the target label is Field, and the reference text has no '.'. Go-gated because C#/Java/C++/Python method bodies legitimately reference their own members bare (cp_reads_writes_cs_static_field pins that shape). Field-validated on the DeusData#1940 stack: USAGE onto Go fields 21308 -> 0, WRITES 5191 -> 0; the only remaining field-targeted edges are 2466 CALLS, which are DeusData#1906/DeusData#1907's selector-guard territory. Reproduce- first pipeline probes (sequential + parallel twins) were RED on the stack without this commit. Fixes DeusData#1942 Signed-off-by: Ilya Brykau <ilya.brykau@orca.security>
The READS/WRITES resolvers and the USAGE registry fallback hand bare reference text to the short-name registry, which contains Field nodes - so once Go struct fields exist (DeusData#1935), every Go local err := ... binds whichever struct field is named err, project-wide: 21308 USAGE and 5191 WRITES onto Go fields on the measured repo, top target a test struct's field T collecting 3013 edges. In Go that binding is impossible by construction: a field is only reachable through a selector expression (x.f), and selector references resolve on the LSP path - every Field-targeted reference edge in the census carried dot-less text. Add cbm_go_suppress_bare_field_ref() next to the DeusData#1928 predicate and consult it at the same four sites (both READS/WRITES resolvers, both USAGE registry fallbacks): drop the bind when the file is Go, the target label is Field, and the reference text has no '.'. Go-gated because C#/Java/C++/Python method bodies legitimately reference their own members bare (cp_reads_writes_cs_static_field pins that shape). Field-validated on the DeusData#1940 stack: USAGE onto Go fields 21308 -> 0, WRITES 5191 -> 0; the only remaining field-targeted edges are 2466 CALLS, which are DeusData#1906/DeusData#1907's selector-guard territory. Reproduce- first pipeline probes (sequential + parallel twins) were RED on the stack without this commit. Fixes DeusData#1942 Signed-off-by: Ilya Brykau <ilya.brykau@orca.security>
|
Reviewed. This is the right shape, and the care in it is above the bar for this class of change. One interaction with your own #1913 needs measuring before either lands. The guard itself
Two decisions in it are better than the guard you mirrored: Keeping Dropping Documenting the residual — a file whose import table maps zero project packages has no penalty signal — instead of implying the class is closed, is the right way to leave it. The measurement I want, and it is about this PR plus #1913These two are in the same stack and both narrow Go call resolution, and I do not think either PR's numbers capture what they do together. Walk one call through both:
So a call that was a confident edge before the stack becomes no edge at all after it — and neither PR sees that on its own. #1913 measures the false edges it removes; this one measures the weak edges it removes. Neither measures the calls that arrive at step 4 only because of step 2. I have asked #1913 for a Go CALLS census before/after with a strategy histogram. What I want here is the same measurement taken on the stack as a whole, not slice by slice: Go edge count and strategy histogram on The per-language census with its dumped per-edge sets is the instrument, and the Go leg is the baseline. None of this is a doubt about the direction. Status
Cleared. Approved on shape, pending the stack-level numbers. |
DeusData
added
bug
A Go selector call x.foo() whose receiver the Go LSP cannot type falls through to the generic registry resolver, which binds it by bare short name to an arbitrary same-named project symbol. Stdlib calls are the worst case: f.Close() on an *os.File gets a CALLS edge to whatever project Close wins candidate ranking (measured on a real Go repo: confidence 0.11, 15 candidates; suffix_match + unique_name were 36% of all CALLS edges, and one 14-line stdlib-only function got 3 out of 3 false outbound edges). Extend the TS/JS receiver-aware guard (DeusData#592/DeusData#606) to Go: - extract_calls.c: flag Go call_expression with a selector_expression callee as is_method, mirroring the TS/JS member_expression flag. - registry.c: add cbm_go_suppress_weak_method_match. Unlike the TS/JS drop-list, field_type_hint is KEPT (Go struct fields carry declared types, so the hint is receiver-aware — lrp_go_s8_field_type_hint), and unique_name is dropped only when its confidence carries the import-unreachability penalty (the stdlib-hijack shape); an unpenalized lone candidate inside the caller's import closure never enters the field-type-hint upgrade and must survive. - pass_calls.c / pass_parallel.c: feed the Go gate next to the TS/JS one; the drop still defers to the emit path so service/route/HTTP edges stay main-identical. Reproduce-first: pipeline_go_receiver_suppresses_weak_method_edge is RED without the extractor flag (the f.Close -> project Close edge exists) and GREEN with it; typed same-package calls, bare local calls and import-qualified cross-package calls still resolve. The old extraction contract test used Go as the flag-exempt language — Python takes that role, and extract_go_selector_call_flags_is_method pins the new behavior. Signed-off-by: Ilya Brykau <ilya.brykau@orca.security>
f76971f to
651ae3c
Compare
|
Rebased onto current main. Only conflict was test insertion-anchor drift in tests/test_pipeline.c (both merges added tests at the same anchor); every source file auto-merged. Full scripts/test.sh (ASan+UBSan) is green on the rebased stack head (#1936, which contains this commit). |
|
Stack-level census is up — full table and per-edge migration matrix posted on #1913 (they share one instrument run, so the numbers live in one place). The short version for the interaction you asked about: The step-2→step-4 cascade is small. Across the #1913 leg, The compounding loss the numbers did surface was elsewhere: a third QN site (go_lsp's enclosing-function QN) that #1913 had not moved, which cost method-body callers their LSP caller join (568 Whole-stack net: 21018 → 13815 Go CALLS, of which −6403 is this PR's weak-edge removal and −1019 is #1936's substring-hint removal; receiver-blind strategy share drops 43% → 10.5%. Stack rebased and force-pushed, full scripts/test.sh green on the head. |
|
Apologies — I assessed this some time ago and never posted the assessment. There is no comment and no review on this PR, so as far as you could tell it had been ignored. That is my error, and here is the review it should have had. Approved on shapeThis is the correct shape for a receiver-precision fix in this project, and it matches every constraint we hold guards to:
Keeping And the Your measurement answers what I was going to ask for: these strategies accounted for 36% of all CALLS edges on the measured repo, and a 14-line stdlib-only helper had 3 out of 3 false outbound edges. That is a census, not an anecdote. Your two reds are ours
Those two setup failures appear in every instance of this red, on three different sections across unrelated changes, which points at one shared setup fault rather than a daemon regression your diff could have caused. Nothing to change. I will merge once that guard is settled or the run comes back clean — and again, sorry for the silence. |
2 participants
What does this PR do?
Fixes #1906.
Extends the TS/JS receiver-aware weak-match guard (#592/#606) to Go: a selector call
x.foo()whose receiver the Go LSP cannot type must not be bound by a receiver-blind short-name strategy to an arbitrary same-named project symbol (f.Close()on an*os.File→ a projectClose; on the repo measured in #1906 these strategies were 36% of all CALLS edges, and one 14-line stdlib-only helper had 3/3 false outbound edges).Mirrors how TS/JS joined the Perl guard (#476):
internal/cbm/extract_calls.c— flag Gocall_expressionwith aselector_expressioncallee asis_method, same block as the TS/JSmember_expressionflag (Go has nothis/superanalog to exempt).src/pipeline/registry.c+pipeline.h— newcbm_go_suppress_weak_method_match(is_go, is_method, strategy, confidence). Dropssuffix_match/fuzzyalways; dropsunique_nameonly when its confidence carries the import-unreachability penalty (<CONF_UNIQUE_NAME) — the stdlib-hijack shape.field_type_hintis deliberately kept: Go struct fields carry declared types, so the hint is receiver-aware there (lrp_go_s8_field_type_hintstays GREEN).src/pipeline/pass_calls.c/pass_parallel.c— feed the Go gate next to the TS/JS one. The drop defers to the emit path exactly like TS/JS, so service/route/HTTP/CONFIG edges stay main-identical.same_module,import_map,qualified_suffix,callee_suffixand alllsp_*strategies are untouched: typed receivers, same-package calls, and import-qualified calls resolve exactly as before.pipeline_go_receiver_suppresses_weak_method_edge— RED without the extractor flag (FAIL: ASSERT(!(cross_file_call_exists(s, project, "FileLen", "Close")))), GREEN with it. Asserts the stdlibf.Close()edge is gone while the typed same-packages.Close(), a bare local call, and an import-qualified cross-package call keep resolving. The fixture carries ago.mod— import reachability (theunique_namepenalty) depends on it, like every real Go repo.extract_go_selector_call_flags_is_methodpins the extractor flag; the old flag-exempt contract test keeps its guard using Python as the exempt language.go_suppress_drops_weak_selector_matches/go_suppress_keeps_typed_and_import_aware_matchesunit-pin the drop-list, the 0.75-vs-0.375unique_namesplit, and thelsp_*keeps.Perf/memory: one
ts_node_child_by_field_name+ twostrcmpper Go call site at extraction, one short-circuited predicate per resolved Go call, zero allocations, no new passes.Known residual (documented in #1906, out of scope here): a Go file whose import table maps zero project packages has no penalty signal, so a lone same-named project symbol can still capture a stdlib method call there;
same_modulecan still mis-bind interface-typed calls inside one package. Both need receiver typing, not name heuristics.Checklist
git commit -s) — required, CI rejectsunsigned commits (DCO, see CONTRIBUTING.md)
scripts/test.sh— full leg, ASan+UBSan, "All tests passed")git clang-format --diff HEADclean on changed lines; clang-tidy/cppcheck via CI)